Skip to content

Fix missing task instance history for non-running retries#67831

Open
GayathriSrividya wants to merge 5 commits into
apache:mainfrom
GayathriSrividya:codex/fix-67238-history
Open

Fix missing task instance history for non-running retries#67831
GayathriSrividya wants to merge 5 commits into
apache:mainfrom
GayathriSrividya:codex/fix-67238-history

Conversation

@GayathriSrividya

@GayathriSrividya GayathriSrividya commented Jun 1, 2026 •
edited by potiuk
Loading

Copy link
Copy Markdown
Contributor

Cherry-picks the proposed fix from #67371 for priority issue #67238.

This updates failure handling to preserve task instance history for retry paths when the task is not in RUNNING state (except RESTARTING, which is already handled).

Also includes the companion test adjustment to avoid detached-session behavior in test_taskinstance.py.

Related: #67238
Related: #65366

Local validation

  • uv run ruff format airflow-core/src/airflow/models/taskinstance.py airflow-core/tests/unit/models/test_taskinstance.py
  • uv run ruff check --fix airflow-core/src/airflow/models/taskinstance.py airflow-core/tests/unit/models/test_taskinstance.py
  • breeze run pytest airflow-core/tests/unit/models/test_taskinstance.py::TestTaskInstance::test_handle_failure_updates_queued_task_updates_state airflow-core/tests/unit/models/test_taskinstance.py::TestTaskInstance::test_task_instance_history_is_created_when_ti_goes_for_retry -xvs (failed locally: Docker not installed)
  • uv run --project airflow-core pytest airflow-core/tests/unit/models/test_taskinstance.py::TestTaskInstance::test_handle_failure_updates_queued_task_updates_state airflow-core/tests/unit/models/test_taskinstance.py::TestTaskInstance::test_task_instance_history_is_created_when_ti_goes_for_retry -xvs (failed locally due missing provider module in environment)

@GayathriSrividya
GayathriSrividya force-pushed the codex/fix-67238-history branch 4 times, most recently from 4f8abb1 to cad74e7 Compare June 2, 2026 08:03
@kalluripradeep @GayathriSrividya
Fix missing task instance history for failed tasks outside of RUNNING…
e39a5c7 
… state

Closes apache#67238
Closes apache#65366

When a task fails from states like QUEUED or DEFERRED, or is failed by the executor or trigger, its state is not RUNNING. The history recording in fetch_handle_failure_context incorrectly assumed that only RUNNING tasks needed their history recorded, which led to missing tries in task_instance_history when CeleryExecutor or KubernetesExecutor failed tasks or triggers failed them. Now, we record history for all states except RESTARTING (since RESTARTING tasks were already cleared and had their history recorded).
@kalluripradeep @GayathriSrividya
Fix detached session object in test_taskinstance.py
fff8a5b
@GayathriSrividya
GayathriSrividya force-pushed the codex/fix-67238-history branch from cad74e7 to fff8a5b Compare June 2, 2026 16:25
@potiuk potiuk added the ready for maintainer review Set after triaging when all criteria pass. label Jun 3, 2026
@potiuk
potiuk marked this pull request as draft July 2, 2026 18:45
@potiuk
potiuk marked this pull request as ready for review July 2, 2026 22:15
@Vamsi-klu

Vamsi-klu commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

The guard flip from ti.state == RUNNING to ti.state != RESTARTING in fetch_handle_failure_context looks right. A retry-eligible failure from QUEUED or DEFERRED never reached prepare_db_for_next_try before, so its history row got silently dropped, and RESTARTING is correctly the one state left out since the clear already recorded it. That lines up with is_eligible_to_retry, where RESTARTING is the always eligible cleared-while-running path. The ti = session.merge(ti) change in test_handle_failure_updates_queued_task_updates_state is right too, since the old line discarded the instance the session actually tracks. I checked that RESTARTING and RUNNING both behave the same under either guard, so this only closes the non-running gap.

@GayathriSrividya

GayathriSrividya commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

The guard flip from ti.state == RUNNING to ti.state != RESTARTING in fetch_handle_failure_context looks right. A retry-eligible failure from QUEUED or DEFERRED never reached prepare_db_for_next_try before, so its history row got silently dropped, and RESTARTING is correctly the one state left out since the clear already recorded it. That lines up with is_eligible_to_retry, where RESTARTING is the always eligible cleared-while-running path. The ti = session.merge(ti) change in test_handle_failure_updates_queued_task_updates_state is right too, since the old line discarded the instance the session actually tracks. I checked that RESTARTING and RUNNING both behave the same under either guard, so this only closes the non-running gap.

Thanks for validating the guard change and the test fix. I’ll wait for formal review and follow up if anything else comes up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@XD-DENG XD-DENG Awaiting requested review from XD-DENG XD-DENG is a code owner
@ashb ashb Awaiting requested review from ashb ashb is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

ready for maintainer review Set after triaging when all criteria pass.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@GayathriSrividya @Vamsi-klu @potiuk @kalluripradeep